Open
Conversation
RobinBol
requested changes
Apr 16, 2026
Split Bitmap class into BitmapBase + Bitmap type alias. The type alias
intersects the base class with a mapped type producing a boolean
property per declared flag, matching the runtime behavior where
Object.defineProperties adds named getters for each flag.
Non-null flag names filter via `K extends string ? K : never` so
`map8('a', null, 'b')` correctly excludes the null slot.
Before: accessing `bitmap.myFlag` errored with TS2339 even though it
works at runtime.
After: `bitmap.myFlag` resolves to `boolean`, matching runtime.
Allow a Struct to be used as a field value inside another Struct definition. Previously the Defs generic was constrained to Record<string, DataType<any>>, which rejected nested Structs with "Type 'StaticStruct<...>' is missing the following properties from type 'DataType<any>': id, shortName, args, defaultValue, ...". Changes: - Introduce StructField = DataType<any> | StaticStruct<any> for the field-value union. - Widen the Defs constraint to Record<string, StructField> on Struct, StaticStruct, StructInstance, and StructProperties. - Extend StructProperties with a second branch: when a field is a StaticStruct<InnerDefs>, it resolves to StructProperties<InnerDefs> recursively, so nested field access narrows correctly. - Change StaticStruct.toBuffer value parameter from StructInstance<Defs> to StructProperties<Defs> so plain nested objects are accepted on write (the common case - you don't need to construct Struct instances just to serialize). The "Structs in Structs are considered a no-go" known-limitation line is removed from the usage example. Nested reads now type as StructProperties (plain shape) instead of StructInstance, so .toJSON() / .toBuffer() on nested fields are no longer typed. This matches runtime: fromBuffer of an outer Struct produces plain nested objects, not inner Struct class instances. Top-level .toJSON()/.toBuffer() are unaffected.
DataTypes.Array8 (and Array0) already accepts a Struct at runtime - arrayToBuf/arrayFromBuf only read .toBuffer, .fromBuffer, and .length from the element type, which both DataType and StaticStruct provide. The .d.ts however only typed the DataType case, so consumers hit "Type 'StaticStruct<...>' is not assignable to parameter of type 'DataType<unknown>'" when passing a Struct. Add an overload matching StaticStruct<Defs> that returns DataType<Array<StructProperties<Defs>>>, so array-of-Struct narrows to an array of plain field-shape objects and plain objects are accepted on write. The DataType overload continues to handle DataTypes.Array8(DataTypes.uint16) etc. unchanged. Also extends the usage example in the doc block with an Array-of-Struct snippet.
index.d.ts had no automated regression coverage. Silent widening or narrowing of a type would only surface when a downstream consumer upgrades and hits a compile error. Add a type-level test file that exercises the public contract, plus CI wiring to run it. - test/types-test.ts: 9 sections covering primitives (numbers, buffers, strings, bool | null, EUI/key), enum narrowing, Bitmap flag property access, Struct basics (create/read/write/typos), nested Structs and arrays, fromBuffer returnLength overload, StaticStruct metadata, encodeMissingFieldsBehavior option, and custom DataType via the exposed constructor. Uses positive type assertions and @ts-expect-error directives so the file fails to compile on any type regression in either direction. - tsconfig.json: strict mode, noEmit, node16 module resolution. Mapped to the in-repo index.d.ts via paths so the tests run against the working copy. - package.json: typecheck script (tsc --noEmit) and typescript devDependency (^5.6.3). - .github/workflows/test.yml: separate Typecheck job on Node 20. The existing test matrix runs Node 12/16 which are too old for TypeScript 5.x (needs Node >= 14.17).
Contributor
|
Pushed 7 commits on top of your PR. Here's what changed: Docs + style (small)
Type fixes (behavior)
New features
Test coverage
Can you review my changes? |
Author
|
Looks good. I hadn't seen the method of overloading used for the Array datatypes before, so it threw me for a bit of a loop, but I also don't know of any way to make that more explicit without cluttering the namespace. Unless you know a clean solution I'll just add a comment for clarity. |
Contributor
|
I think a comment is fine! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The data types did not yet reflect the actual type of data they contained, so I made the class definition generic and added a new DataTypes definition using it.
I also took the liberty to rewrite the other definitions based on the types we have built at @athombv/drenso, based on the JS source and runtime introspection.